feat: make foreground service opt-in#1053
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9a98728be
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
ovitrif
left a comment
There was a problem hiding this comment.
Reviewed locally. I left two comments: one lifecycle issue around the background transition, and one changelog cleanup. Leaving this as a neutral review.
| Lifecycle.Event.ON_STOP -> { | ||
| if (walletExists && !isRecoveryMode && !notificationsGranted) { | ||
| // App backgrounded without notification permission - stop node | ||
| val keptAliveByService = notificationsGranted && keepActiveInBackground |
There was a problem hiding this comment.
I think this observer can use stale notificationsGranted and keepActiveInBackground values because the effect is keyed only by lifecycle. If the user toggles Keep Bitkit active during the same activity session, the next ON_STOP can still follow the old setting and either stop the node despite the service being enabled or leave it running after the service was disabled. Could we keep these values current inside the observer, for example with rememberUpdatedState or by recreating the effect when they change?
| @@ -0,0 +1 @@ | |||
| Background payments now include a "Keep Bitkit active in background" option for more reliable payments. | |||
There was a problem hiding this comment.
Could we rename this fragment to 1053.added.md and update the text to also mention that payment notifications now always include the amount? The collection script only appends the PR reference when the fragment ref is numeric, and this PR removes the previous hidden-amount behavior.
Fixes #1007
This PR:
Description
Preview
disable-fg-switch.webm
disable-notifications.webm
running-from-master.webm
wake-to-pay.webm
enable-fg-service.webm
QA Notes
FIGMA
Manual Tests
regression:Keep active off, app closed → receive a payment: push notification arrives and shows the amount.Automated Checks
SettingsViewModelTest.kt.ReceivedNotificationContentTest.ktandNotifyChannelReadyHandlerTest.ktbecause the show-notification-details option no longer exists;SettingsDatastubs updated inNotifyPaymentReceivedHandlerTest.kt.just compile, the affected unit tests, andjust lint.